Add gc option and nil for grace_time option #4
Conversation
There was a problem hiding this comment.
Thank you for enjoying this gem. And thank you even more for participating in it!
I made a few remarks in the code that are hints for tiny enhancements, but the way your implementation is made is clean enough in my opinion.
However, the grace_time: nil option feels a bit weird IMHO. I mean, say you have an infinite job that takes all your resources, you would maybe like sidekiq to wait a lot but at some point, it should restart anyhow. You could then have a really long time set instead of nil (1.day.to_i for instance).
lib/sidekiq/worker_killer.rb
Outdated
| @@ -12,17 +12,20 @@ class WorkerKiller | |||
|
|
|||
| def initialize(options = {}) | |||
| @max_rss = (options[:max_rss] || 0) | |||
| @grace_time = (options[:grace_time] || 15 * 60) | |||
| @grace_time = (options.key?(:grace_time) ? options[:grace_time] : 15 * 60) | |||
There was a problem hiding this comment.
I think it would be better to have a consistent way of initializing variables here. This said I'd lean more in favor of the #key? option since it avoids unexpected behavior such as:
expect_false_but_obviously_is_true = false || true
# This would impact the `gc` option for instanceWhat do you think?
There was a problem hiding this comment.
I think you are right. It is my bad. Best option is options.fetch(:grace_time, 15 * 60), and it is supported since Ruby 2.0.0. I'll change to consisted way here.
spec/sidekiq/worker_killer_spec.rb
Outdated
| expect(GC).to receive(:start).with(full_mark: true, immediate_sweep: true) | ||
| subject.call(worker, job, queue){} | ||
| end | ||
| context "but gc is false" do |
There was a problem hiding this comment.
In our codebase, we tend to prefix context elements with when.
| context "but gc is false" do | |
| context "when gc is false" do |
I agreed that it is weird. My assumption is you have no infinite jobs in the application. It is very useful for my case. |
There was a problem hiding this comment.
Okay, let's go for that option. Since this isn't the default behavior, it should mean that someone that sets grace_time: nil knows what he is doing.
@aishek, could you do a last rubocop check / commit squash before I merge?
Thank you very much for this improvement 🎉
|
I completely agree with However, seems to me than setting |
Good evening and thanks for useful gem!
I've got two special use cases for the gem: restart as soon as possible, and wait all jobs to finish without timeout. So I implemented
gc: falseoption andgrace_time: nil.How it looks like? What should I change?